Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(sdk-metrics-base): meter identity #2901

Merged
merged 2 commits into from
Apr 19, 2022

Conversation

legendecas
Copy link
Member

Which problem is this PR solving?

As spec defines:

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#get-a-meter
Meters are identified by all of these fields. When more than one Meter of the same name, version, and schema_url is created, it is unspecified whether or under which conditions the same or different Meter instances are returned. The term identical applied to Meters describes instances where all identifying fields are equal. The term distinct applied to Meters describes instances where at least one identifying field has a different value.

Also said:

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/datamodel.md#opentelemetry-protocol-data-model-producer-recommendations
Producers SHOULD prevent the presence of multiple Metric identities for a given name with the same Resource and Scope attributes. Producers are expected to aggregate data for identical Metric objects as a basic feature, so the appearance of multiple Metric, considered a "semantic error", generally requires duplicate conflicting instrument registration to have occurred somewhere.

So we need to not create a new MeterSharedState when a given name+version+schemaUrl pair is already created in the MeterProvider.

Fixes Partial #2593

Short description of the changes

  • MeterProviderSharedState stores a map of MeterSharedState, keyed with the name+version+schemaUrl pair.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • MeterProvider.getMeter

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added

@legendecas legendecas requested a review from a team April 18, 2022 06:13
@legendecas legendecas force-pushed the metrics-ff/meter-identity branch from 569a402 to af786dd Compare April 18, 2022 06:14
@codecov
Copy link

codecov bot commented Apr 18, 2022

Codecov Report

Merging #2901 (9e560b7) into main (ed2f033) will increase coverage by 0.03%.
The diff coverage is n/a.

❗ Current head 9e560b7 differs from pull request most recent head 3508a7f. Consider uploading reports for the commit 3508a7f to get more accurate results

@@            Coverage Diff             @@
##             main    #2901      +/-   ##
==========================================
+ Coverage   92.48%   92.51%   +0.03%     
==========================================
  Files         151      151              
  Lines        4841     4848       +7     
  Branches     1022     1024       +2     
==========================================
+ Hits         4477     4485       +8     
+ Misses        364      363       -1     
Impacted Files Coverage Δ
...ckages/opentelemetry-sdk-metrics-base/src/Meter.ts 100.00% <0.00%> (ø)
...ckages/opentelemetry-sdk-metrics-base/src/utils.ts 100.00% <0.00%> (ø)
...pentelemetry-sdk-metrics-base/src/MeterProvider.ts 100.00% <0.00%> (ø)
...etry-sdk-metrics-base/src/state/MetricCollector.ts 100.00% <0.00%> (ø)
...try-sdk-metrics-base/src/state/MeterSharedState.ts 100.00% <0.00%> (ø)
...metrics-base/src/state/MeterProviderSharedState.ts 100.00% <0.00%> (ø)
...emetry-core/src/platform/node/RandomIdGenerator.ts 93.75% <0.00%> (+6.25%) ⬆️

Copy link
Contributor

@seemk seemk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

Comment on lines +38 to +39
const meterCollectionPromises = Array.from(this._sharedState.meterSharedStates.values())
.map(meterSharedState => meterSharedState.collect(this, collectionTime));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Random trivia: Array.from(states.values(), state => state.collect(this, collectionTime)); would be shorter, but benched it for fun and found that Array.from(states.values()).map(state => state.collect(this, collectionTime)); is ~4x faster on Node.js 🤷‍♂️

@legendecas
Copy link
Member Author

@dyladan updated, please take a look again :)

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good but I think my second suggestion for changelog wording was actually more accurate than the first. The implementation looks fine to me though.

@legendecas
Copy link
Member Author

@dyladan now that the implementation does return the same meter object on identical input, the changelog entry should be fine I think? As the "shared internal state" is not exposed to end-users, I'd find your first suggestion is good though.

@dyladan
Copy link
Member

dyladan commented Apr 18, 2022

@dyladan now that the implementation does return the same meter object on identical input, the changelog entry should be fine I think? As the "shared internal state" is not exposed to end-users, I'd find your first suggestion is good though.

I missed the meter trailing on line 119. Looks good to me.

@legendecas
Copy link
Member Author

thank you for reviewing :)

@legendecas legendecas merged commit 2ba6dd5 into open-telemetry:main Apr 19, 2022
@legendecas legendecas deleted the metrics-ff/meter-identity branch April 19, 2022 02:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants